-
Notifications
You must be signed in to change notification settings - Fork 204
feat(search): collapsed search #4115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(search): collapsed search #4115
Conversation
🦋 Changeset detectedLatest commit: 62498f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
File metricsSummaryTotal size: 1.43 MB*
search
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
📚 Branch previewPR #4115 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4115/index.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this. I feel like I still mainly have questions, so I left a few for you 😊
I might consider just updating the changeset heading to "Collapsed" instead of "Minimized." Just personal preference though.
Have you reached out to design to ask about some of the focus and keyboard focus behaviors? I found the collapsed usage section in Figma, and the "focus state" of this button is just...open? That feels odd to me for some reason. Like if I was tabbing along, does it make sense for me to tab to this action button, and then it just be a completely different component? Anyways, I think this is more along the lines that I would expect, where I'm tabbing along, and I have to trigger the rest of the search field to open. I may be wrong though. Is that worth bringing up with design?
components/search/index.css
Outdated
|
||
/* Animation for collapsible search expansion */ | ||
&.is-collapsed { | ||
transition: inline-size 0.3s cubic-bezier(0.4, 0, 0.2, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you feel about maybe using of any of the --spectrum-animation-duration-*
tokens? Maybe we can use that here? Or was the 0.3 design directed? And just because I'm nosy, where did you find these timing curves?!
Should the cubic-beziers and even the transition-duration be set as custom props? Maybe it's not necessary, but we do use the same curve & durations twice 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just experimenting with animation curves here. I don't know what the intended animation should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice! I've been learning about each of the values of the cubic-bezier
function, so I was just curious!
customClasses: [ | ||
`${rootClass}-actionButton`, | ||
isHovered && "is-hover", | ||
isDisabled && "is-disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only suggestion is to make sure we can't keyboard focus to this button when it's disabled!

I'm not sure if this is a property thing? It certainly looks like we're passing the disabled arg through correctly- I see it on the search AND on the action button, but maybe we need to revisit the disabled+keyboard focus styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful!
Not technically a blocker, but we should make sure to continue defining a custom prop to use in our styles for consistency and maintainability. Instead of using --spectrum-animation-duration-800
directly in the styles, we typically create a component-specific custom prop that is set to animation-duration-800
. It would help in the long run if that spec needs to change, we'd only have to update it once in the custom props at the top of the file, instead of multiple times throughout the file.
Again, not technically a blocker, but I'm a sucker for consistency 🥴
Description
The search component allows for a minimized state where the search field is collapsed to an action button. When clicked the the search field is visible.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
Include steps for the PR reviewer that explain how they should test your PR. Example test outline:
true
is collapsed state (@cdransf)Regression testing
Validate:
Screenshots
States
Accessibility
Keyboard Navigation
Tab
key to access the search button and pressEnter
to expand the search fieldTo-do list